Feature/grid filter continued#269
Open
santipalenque wants to merge 5 commits into
Open
Conversation
📝 WalkthroughWalkthroughExtends the GridFilter component with five new value input types: ChangesGridFilter new value types, async select, and setFilters API
Sequence Diagram(s)sequenceDiagram
participant Filter
participant ValueInput
participant AsyncSelectInput
participant queryFunction
participant GridFilter_parseFilter
Filter->>ValueInput: render(type="company", value, options)
ValueInput->>AsyncSelectInput: delegate via INPUT_TYPE_MAP
AsyncSelectInput->>queryFunction: queryFunction("", callback) on mount
queryFunction-->>AsyncSelectInput: callback(companies)
AsyncSelectInput->>Filter: onChange({target:{value:{value,label,raw}}})
Filter->>GridFilter_parseFilter: handleSubmit(filters)
GridFilter_parseFilter->>GridFilter_parseFilter: criteria=criterias[filter.criteria]
GridFilter_parseFilter->>GridFilter_parseFilter: check ASYNC_VALUE_TYPES.includes(criteria.type)
alt customParser missing
GridFilter_parseFilter->>GridFilter_parseFilter: console.error
else customParser provided
GridFilter_parseFilter->>GridFilter_parseFilter: return customParser(filter)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/Dropdown/index.jsx (1)
42-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd optional chaining to prevent crash when options is null.
Now that
optionscan benull(per the newdefaultPropsat line 99), line 52 and line 59 are missing the optional chaining guard that line 65 already has.Crash scenario for line 52:
- Dropdown receives
multiple={true}via the{...rest}spread (line 41)valueis an array (e.g.,[1, 2])optionsisnullrenderValuereaches line 50'sArray.isArray(selected)check → true- Line 52 executes
options.map(...)→ runtime crash: "Cannot read property 'map' of null"Crash scenario for line 59:
- Single-select mode (default)
valueis a non-empty scalaroptionsisnull- Line 59 executes
options.find(...)→ runtime crash🛡️ Proposed fix
if (Array.isArray(selected)) { const lookup = Object.fromEntries( - options.map((o) => [o.value, o.label]) + options?.map((o) => [o.value, o.label]) || [] ); return selected .map((v) => lookup[v]) .filter(Boolean) .join(", "); } - const selectedOption = options.find( + const selectedOption = options?.find( ({ value }) => value === selected );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/Dropdown/index.jsx` around lines 42 - 63, The renderValue function in the Dropdown component is missing optional chaining guards on the options parameter. At line 52 where options.map is called to create the lookup object for array values, and at line 59 where options.find is called to find the selected option label, add optional chaining (the ?. operator) before the .map() and .find() method calls respectively. This will prevent runtime crashes when options is null by ensuring these methods are only called if options exists.
🧹 Nitpick comments (1)
src/components/mui/GridFilter/utils.js (1)
31-32: Operator value collision: affects semantic preservation on deserialization.
BEFOREandAFTERreuse the same operator values ("<="and">=") asLESS_OR_EQUALandGREATER_OR_EQUAL. TheparseFilterfunction serializes filters using only the operator value string (e.g.,"fieldName<=value"), with no way to reconstruct which semantic intent was originally selected. On filter deserialization, a filter created withBEFOREbecomes indistinguishable from one created withLESS_OR_EQUAL, losing temporal context.If this aliasing is intentional (semantic labels for date-specific contexts), document the design. Otherwise, consider using distinct values (e.g.,
"<="for numeric,"<@"for temporal) to preserve semantic information.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/mui/GridFilter/utils.js` around lines 31 - 32, The BEFORE and AFTER operators in the grid filter operators object are using the same operator values as LESS_OR_EQUAL and GREATER_OR_EQUAL respectively, causing operator value collision. When filters are serialized and later deserialized through the parseFilter function, there is no way to distinguish whether a filter was originally created with BEFORE or LESS_OR_EQUAL (both use "<="), losing the semantic temporal context. Change the operator values for BEFORE and AFTER to use distinct values that do not collide with numeric comparison operators (for example, use "<@" for BEFORE and ">@" for AFTER instead of reusing "<=" and ">=") to preserve semantic information during serialization and deserialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsx`:
- Around line 56-74: The fetchOptions function has a race condition where slower
earlier requests can overwrite newer results when the user types quickly, and
callbacks can also update state after unmount. Add a ref to track the mounted
status and another ref to track the current request ID. In the useEffect, set
mounted to true and clear it in the cleanup function. Modify fetchOptions to
generate a unique request ID for each call and pass it to the queryFunction
callback. In the callback, before calling setOptions and setLoading, check that
the request ID matches the current request ID stored in the ref and that the
component is still mounted. This ensures only the latest active request mutates
state and prevents updates after unmount.
---
Outside diff comments:
In `@src/components/mui/Dropdown/index.jsx`:
- Around line 42-63: The renderValue function in the Dropdown component is
missing optional chaining guards on the options parameter. At line 52 where
options.map is called to create the lookup object for array values, and at line
59 where options.find is called to find the selected option label, add optional
chaining (the ?. operator) before the .map() and .find() method calls
respectively. This will prevent runtime crashes when options is null by ensuring
these methods are only called if options exists.
---
Nitpick comments:
In `@src/components/mui/GridFilter/utils.js`:
- Around line 31-32: The BEFORE and AFTER operators in the grid filter operators
object are using the same operator values as LESS_OR_EQUAL and GREATER_OR_EQUAL
respectively, causing operator value collision. When filters are serialized and
later deserialized through the parseFilter function, there is no way to
distinguish whether a filter was originally created with BEFORE or LESS_OR_EQUAL
(both use "<="), losing the semantic temporal context. Change the operator
values for BEFORE and AFTER to use distinct values that do not collide with
numeric comparison operators (for example, use "<@" for BEFORE and ">@" for
AFTER instead of reusing "<=" and ">=") to preserve semantic information during
serialization and deserialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87ce6b08-1d5e-4fb1-b5ad-f9aefa59e15b
📒 Files selected for processing (13)
src/components/mui/Dropdown/index.jsxsrc/components/mui/GridFilter/GridFilter.jsxsrc/components/mui/GridFilter/components/Filter.jsxsrc/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/CompanySelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/DateTimeInput.jsxsrc/components/mui/GridFilter/components/ValueInput/NumberInput.jsxsrc/components/mui/GridFilter/components/ValueInput/SpeakerSelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/index.jsxsrc/components/mui/GridFilter/readme.mdsrc/components/mui/GridFilter/utils.jssrc/components/mui/__tests__/GridFilter.test.jsxsrc/i18n/en.json
Comment on lines
+56
to
+74
| const fetchOptions = (searchTerm) => { | ||
| if (searchTerm && searchTerm.length < minSearchLength) { | ||
| setOptions([]); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| queryFunction(searchTerm, (rawResults) => { | ||
| setOptions((rawResults || []).map((item) => ({ ...formatOption(item), raw: item }))); | ||
| setLoading(false); | ||
| }); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetchOptions(""); | ||
| return () => { | ||
| if (debounceRef.current) clearTimeout(debounceRef.current); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Guard against stale/out-of-order async responses when typing quickly.
fetchOptions accepts every callback result, so slower earlier requests can overwrite newer results, and callbacks can still update state after unmount. Track a request id + mounted flag so only the latest active request mutates state.
💡 Suggested fix
@@
const [options, setOptions] = useState([]);
const [loading, setLoading] = useState(false);
const debounceRef = useRef(null);
+ const lastRequestIdRef = useRef(0);
+ const isMountedRef = useRef(true);
@@
const fetchOptions = (searchTerm) => {
+ const requestId = ++lastRequestIdRef.current;
if (searchTerm && searchTerm.length < minSearchLength) {
setOptions([]);
+ setLoading(false);
return;
}
setLoading(true);
queryFunction(searchTerm, (rawResults) => {
+ if (!isMountedRef.current || requestId !== lastRequestIdRef.current) return;
setOptions((rawResults || []).map((item) => ({ ...formatOption(item), raw: item })));
setLoading(false);
});
};
@@
useEffect(() => {
fetchOptions("");
return () => {
+ isMountedRef.current = false;
if (debounceRef.current) clearTimeout(debounceRef.current);
};Also applies to: 76-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsx`
around lines 56 - 74, The fetchOptions function has a race condition where
slower earlier requests can overwrite newer results when the user types quickly,
and callbacks can also update state after unmount. Add a ref to track the
mounted status and another ref to track the current request ID. In the
useEffect, set mounted to true and clear it in the cleanup function. Modify
fetchOptions to generate a unique request ID for each call and pass it to the
queryFunction callback. In the callback, before calling setOptions and
setLoading, check that the request ID matches the current request ID stored in
the ref and that the component is still mounted. This ensures only the latest
active request mutates state and prevents updates after unmount.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 3: The `@mui/x-date-pickers` package is currently listed only in
devDependencies but needs to be added to peerDependencies as well, since the new
DateTimeInput component (which uses DateTimePicker from this package) is
exported as part of the GridFilter component library. Add `@mui/x-date-pickers` to
the peerDependencies section in package.json with the same version constraint as
what is currently specified in devDependencies, so that consuming applications
receive clear guidance about this required dependency when using GridFilter with
the datetime value type.
In `@src/components/mui/GridFilter/hooks/useGridFilter.jsx`:
- Around line 29-30: The setFilters function on line 29-30 accepts any
joinOperator value without validation before dispatching to saveFilters, which
allows invalid values to be persisted in state and cause silent semantic drift
in the filter-reducer. Add validation logic inside setFilters to ensure the
joinOperator parameter is clamped to only valid values from JOIN_OPERATORS
(specifically ALL or ANY) before passing it to the dispatch call, using a safe
default value if an invalid value is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cbf0bec-fe2e-4eb4-9eb6-95aa3c0796d9
📒 Files selected for processing (4)
package.jsonsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/GridFilter/readme.mdsrc/components/mui/__tests__/useGridFilter.test.jsx
| { | ||
| "name": "openstack-uicore-foundation", | ||
| "version": "5.0.34", | ||
| "version": "5.0.36-beta.1", |
There was a problem hiding this comment.
Missing @mui/x-date-pickers in peerDependencies.
The new DateTimeInput component uses DateTimePicker from @mui/x-date-pickers, but this package is only listed in devDependencies (line 30), not in peerDependencies. Since this is a component library that exports GridFilter to consuming applications, @mui/x-date-pickers must be declared as a peer dependency.
Without this declaration, consuming applications that use GridFilter with the new datetime value type will encounter runtime errors when DateTimeInput attempts to import DateTimePicker, and developers won't receive clear guidance from package.json about the missing dependency.
📦 Proposed fix to add missing peer dependency
"peerDependencies": {
"`@emotion/react`": "^11.11.4",
"`@emotion/styled`": "^11.11.5",
"`@mui/icons-material`": "^6.4.3",
"`@mui/material`": "^6.4.3",
+ "`@mui/x-date-pickers`": "^7.26.0",
"`@react-pdf/renderer`": "^3.1.11",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 3, The `@mui/x-date-pickers` package is currently listed
only in devDependencies but needs to be added to peerDependencies as well, since
the new DateTimeInput component (which uses DateTimePicker from this package) is
exported as part of the GridFilter component library. Add `@mui/x-date-pickers` to
the peerDependencies section in package.json with the same version constraint as
what is currently specified in devDependencies, so that consuming applications
receive clear guidance about this required dependency when using GridFilter with
the datetime value type.
Comment on lines
+29
to
+30
| const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) => | ||
| dispatch(saveFilters(id, filters, joinOperator)); |
There was a problem hiding this comment.
Validate joinOperator before dispatching.
Line 29 forwards any joinOperator value into state; an invalid value is persisted and skips the JOIN_OPERATORS.ANY path in filter-reducer, so parsed semantics can silently drift. Clamp to ALL | ANY at this API boundary.
Suggested patch
- const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) =>
- dispatch(saveFilters(id, filters, joinOperator));
+ const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) => {
+ const safeJoinOperator =
+ joinOperator === JOIN_OPERATORS.ANY
+ ? JOIN_OPERATORS.ANY
+ : JOIN_OPERATORS.ALL;
+ dispatch(saveFilters(id, filters, safeJoinOperator));
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/GridFilter/hooks/useGridFilter.jsx` around lines 29 - 30,
The setFilters function on line 29-30 accepts any joinOperator value without
validation before dispatching to saveFilters, which allows invalid values to be
persisted in state and cause silent semantic drift in the filter-reducer. Add
validation logic inside setFilters to ensure the joinOperator parameter is
clamped to only valid values from JOIN_OPERATORS (specifically ALL or ANY)
before passing it to the dispatch call, using a safe default value if an invalid
value is provided.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86baf0qnm
Summary by CodeRabbit
Release Notes
before/afteroperators.datetime,number(min/max + integer handling), and async selects forspeakerandcompany.useGridFilternow exposessetFilters(filters, joinOperator)to programmatically populate filter state.